I wonder if for all potential elements, xmax and ymax are general enough argument names.

They do make sense for the Aperture element, but for all other thick elements we might need something more specific, e.g., x_open or x_aperture?

Another small change I thought we could do is use std::optional<amrex::ParticleReal> ... = std::nullopt. That way, we do not redefine 0 with a jump, which technically is a fully blocking aperture. The equivalent in Python is then x_aperture = None, which is clearly no aperture.

What do you think?

I like x_aperture, and I'll make that change. Will look into using std::optional.

To get the value out of std::optional, call operator* or .value().

We will need to write the real value into m_x_aperture because we cannot (should not) use std::optional on GPU. For no value, I would maybe write a negative value into the member variable and only apply the aperture for >=0. That way, 0 is well-defined (blocking).

To clarify the last comment: We're avoiding std::optional altogether? If so, I actually prefer the existing approach, to apply the aperture when x_aperture > 0 and y_aperture >0. Why? 1) Allowing for blocking with x_aperture = 0 requires changing the logic/math in the mixin class, 2) A thick element with zero aperture has no use case (eg, its action is the same as a thin screen for blocking), 3) Allowing this case means setting the default values of x_aperture, y_aperture to an arbitrarily-chosen negative number.

Ok, then we can keep it simple and document <= zero as no-aperture 👍

Oh, breaking change for existing aperture element users.

Not necessarily needed for this PR to work, but we can make it work by adding a bit more logic to transition.

Yes, I agree. The case for consistent naming is pretty strong. One of the main aperture users is @SchroederSa, who was the first to suggest that xmax and ymax were not the best names. Did you have in mind to support both input syntax by modifying InitElement.cpp and python/elements.cpp?

Yes, I'll add a fallback to make the transition easier there.

If we want to rename the parameter (not needed for this PR), then I would add a bool has_xmax = pp_element.query("xmax", xmax); then if true, throw a warning and set the value for aperture_x (unless that one is set as well). Same for y.

But as I said, I don't think we need to rename the Aperture arguments with this PR.

Still have to add this part.

As above: we could avoid this breaking change for the Aperture element itself.

The break of API arg name here might be ok, for python people might not use the named arguments yet that much.